-
Notifications
You must be signed in to change notification settings - Fork 458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fleet UI: Add timestamps to host count on software detail pages #25143
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #25143 +/- ##
=======================================
Coverage 63.85% 63.85%
=======================================
Files 1617 1618 +1
Lines 153841 153846 +5
Branches 3944 3996 +52
=======================================
+ Hits 98231 98236 +5
+ Misses 47796 47795 -1
- Partials 7814 7815 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks fine (though it feels like we could extract the host count tooltip message to a constant somewhere), but styling is a bit off. Which probably means tweaking the last-updated counts element.
From Figma:
Checked the font on that item in Figma and it's showing bold rather than standard, and I'm 99% sure the font-weight on the added items isn't bold. Additionally, seems like we're missing a tiny bit of left-pad
ing on the count element to match up with Figma (and it feels like we do need that padding). Can't eyeball this exactly (I probably could if host count was 48) but maybe 4-8px?
@iansltx thanks for catching these, I just did what the default was for current styling
Example of other updated at text not being aligned baseline and not being bold: |
Looking at the live software pages it looks like "Updated" is bold:
Which is probably why I made it bold here. That said, I don't think it needs to/should be as, as you point out, it isn't elsewhere in the platform. I def like it better unbolded. I'll make the changes to unbold in the associated figma file. Thanks for calling that out!
It's actually been annoying me that text isn't baseline aligned in the platform. I'm baseline aligning it on the pages I touch and def would like to see it baseline aligned throughout going forward. It looks a little like a mistake (or that the tooltip underline is pushing the text up) when it's not baseline aligned imho. |
@eugkuo - sweet, I agree. Maybe let's make a bug ticket for this so we can track fixing it everywhere. I'll push it aligned baseline here to get that ball rolling. I'll unbold the text and push up to this PR |
@@ -1,5 +1,6 @@ | |||
.component__last-updated-text { | |||
font-size: $xx-small; | |||
font-weight: $regular; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated globally here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Rachel. Can you also please remove these lines (same fix, locally) with merging this global update
@RachelElysia I've created this bug ticket. I'll populate it with figma links in the "To fix" section tonight: #25180 |
f737a91
to
8dc8ec7
Compare
@iansltx ready for ur review <3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LastUpdatedHostCount
feels a little bit weird naming-wise, vs. e.g. HostCountWithFreshness
, but my opinion on this isn't strong enough to block. Otherwise code LGTM; thanks for the cleanup/componentization!
Issue
For #24975
Description
Screenshot of all 3
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/
,orbit/changes/
or